-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase auth nonce entropy #1383
Conversation
Pull Request Test Coverage Report for Build 8627601648Details
💛 - Coveralls |
* @see https://github.com/StableLib/stablelib/blob/5243520e343c217b6a751464dec1bc980cb510d8/packages/random/random.ts#L80-L99 | ||
*/ | ||
|
||
private static readonly NONCE_LENGTH = 17; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generates 34 hex characters strings, as nonce
if I'm not wrong. The implementation referenced above produces 96 bits (so 48 hex characters), right? I don't have an opinion about the size, both seem enough, but maybe we can add a comment documenting the decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment is not clear enough (I tried to simplify/extract explicit code from the example packages). Is there something in particular I could adjust to make it more understandable?
const ALPHANUMERIC = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
const length = Math.ceil(96 / (Math.log(ALPHANUMERIC.length) / Math.LN2)) // 17 characters long
Note: I tested the official package again and calling generateNonce
of siwe
returned the following: "UhC3yE7xXrVU6t0Qs"
which is 17 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, thanks for explaining, the length
matches the official package one then 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into our implementation and you're correct. I'll mark this as a draft and revisit this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the nonce length to 18 as of b2ccdf2.
As we're using the typed array to generate random values, we can't use 17 as we'd need 8.5 elements. I think it's better to stick with 18 as it's a) more secure and b) simpler than then removing a character after generation, for example. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes perfect sense to me, I think we are free to set the length that fits better here, 18 seems OK to me.
Summary
This increases the nonce entropy the auth implementation to match that of the official SiWe implementation (18 characters).